Skip to content

Add missing function getsocketoption #7361

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

Add missing function getsocketoption #7361

wants to merge 6 commits into from

Conversation

rtpmsys
Copy link
Contributor

@rtpmsys rtpmsys commented Oct 15, 2022

WiFimanager.cpp connect function truncated all connnect timeout values below 1000ms to 0ms and rounded timeouts to the full second, e.g. 2.5 seconds became 2 seconds. For timeouts below 1000ms, the connect function failed. This fix handles timeouts correctly, by putting the fractional part of the timeout into the timeout_us variable.

Added function getsocketoptions, it was missing.

@rtpmsys
Update WiFiClient.h
e204e7b 
Added missing getSocketOption() with full access to level and option
@rtpmsys
Update WiFiClient.h
bcc50bf
@rtpmsys
Merge pull request #2 from rtpmsys/rtpmsys-getsocketoption-1
137c21e 
Update WiFiClient.h
@rtpmsys
Merge pull request #1 from rtpmsys/rtpmsys-getsocketoption
c8c2076 
Update WiFiClient.h
@rtpmsys
Handle timeout settings below 1 second
ed42643 
Fix error with connect timeout settings below 1000ms.
Add getsocketoptions function.
@rtpmsys
Merge branch 'espressif:master' into master
40da8af
@VojtechBartoska VojtechBartoska added the Area: BT&Wifi BT & Wifi related issues label Oct 21, 2022
@me-no-dev
Copy link
Member

This PR seems to include two non-relevant changes. Could you please split the timeout fix and getSocketOption into separate PRs?

@VojtechBartoska
Copy link
Contributor

Any updates @rtpmsys?

@TD-er
Copy link
Contributor

TD-er commented Dec 27, 2022 •
edited
Loading

Why keep on insisting to have a WiFiClient::setTimeout function in this class using seconds while every other piece of code uses msec?
Cast it to Client and call that timeout will expect msec., which is a clear indication this design choice in the past was wrong.

@rtpmsys
Copy link
Contributor Author

rtpmsys commented Jan 9, 2023 •
edited
Loading

I submitted only the timeout correction and omitted the

This PR seems to include two non-relevant changes. Could you please split the timeout fix and getSocketOption into separate PRs?

I submitted just the timeout change (#7686) which fixes the error with connect timeout values below 1 second.

@rtpmsys
Copy link
Contributor Author

rtpmsys commented Jan 9, 2023 •
edited
Loading

Any updates @rtpmsys?

I submitted a new pull request (#7686) with just the timeout fix. Hope it makes it through the system, now.

@rtpmsys
Copy link
Contributor Author

rtpmsys commented Jan 9, 2023 •
edited
Loading

Why keep on insisting to have a WiFiClient::setTimeout function in this class using seconds while every other piece of code uses msec?
Cast it to Client and call that timeout will expect msec., which is a clear indication this design choice in the past was wrong.

You are right that the setTimeout function only expects seconds, I guess for compatibility reasons? The setTimeout luckily only seems to affect the write and read from the socket, not the connect.
My proposed fix (#7686) deals with the connect timeout, which comes before the write or read, therefore it is much more important that it can be made smaller than 1 second.

@mrengineer7777
Copy link
Collaborator

@rtpmsys #7686 was merged. Time to update this PR?

@rtpmsys rtpmsys changed the title Fix WiFiClient to handle connect timeout < 1000ms properly Add missing function getsocketoptions Feb 6, 2023
rtpmsys
rtpmsys commented Feb 6, 2023
View reviewed changes
libraries/WiFi/src/WiFiClient.cpp
@@ -231,7 +231,7 @@ int WiFiClient::connect(IPAddress ip, uint16_t port, int32_t timeout)
FD_ZERO(&fdset);
FD_SET(sockfd, &fdset);
tv.tv_sec = _timeout / 1000;
tv.tv_usec = 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved into another PR

@rtpmsys rtpmsys changed the title Add missing function getsocketoptions Add missing function getsocketoption Feb 6, 2023
@rtpmsys rtpmsys closed this Feb 6, 2023
@rtpmsys
Copy link
Contributor Author

rtpmsys commented Feb 6, 2023

I will submit a new PR for adding the missing function getSocketOption.

@rtpmsys rtpmsys mentioned this pull request Feb 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Area: BT&Wifi BT & Wifi related issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rtpmsys @me-no-dev @VojtechBartoska @TD-er @mrengineer7777